Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

James Taylor tech-test #8

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jamestaylordev
Copy link

@jamestaylordev jamestaylordev commented Aug 10, 2019

James Taylor: tech-test solution

Notes

  1. I haven't implemented any external interface into the system. I didn't want to make any assumptions around how this system is consumed so the integration tests should demonstrate that the solution works end to end for the given requirements.
  2. I've included scripts for scaffolding the database in the Database Scaffolding Scripts solution folder. I made these scripts idempotent so they can be run over any existing database. Bundled in these scripts is also a migration to add CustomerId to theOrders.dbo.Orders table - I made this a safe change, but it doesn't really follow a real world example of how I'd apply a migration since there isn't a data migration to go with it.
  3. I ran a bit thin on time - I went for a TDD approach and would have add more tests for all scenarios if I could have.

General Approach

  1. Initially I created the database and scaffolding scripts and got a happy path integration test running around the existing OrderService. This allowed me to make sure the refactoring work I was doing wasn't going to introduce breaking changes.
  2. I decided to introduce Dtos to be returned/passed into the service layer. I did this to encapsulate the domain model and associated business logic.
  3. I decided to split the DAL into contract and implementation projects - this was just to ensure the entire (or parts of) the data layer could be reimplemented without changing the service layer. In real life I would probably use some kind of generic IRepository<T> pattern, but felt that was a bit overkill for this solution.
  4. I chose to use Dapper as an ORM as it's quite light weight and just simplifies the ADO.NET querying. I considered using EntityFramework, but felt Dapper lent itself better to working with the existing database and this small system.
  5. I kept the static CustomerRepository wrapping it behind a CustomerRepositoryWrapper - I assumed this static class to be some kind of legacy we couldn't immediately change the signature of so though it was best to abstract it for reimplementation at a later date.
  6. I noticed the Orders.OrderId table isn't an auto incrementing primary key - I assumed this was by design so kept it this way - hence a bit of convoluted logic to generate OrderIds in the integration tests.
  7. I used static mapper classes for mapping between the Service Layer/DAL - this was just to remove mapping logic from the main methods - in a real world example I would probably use Automapper
  8. I added CastleWindsor for IoC - this is only used in the integration tests so is maybe slightly overkill, but I felt it kept them cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant